-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rm sponsored, verified #22671
rm sponsored, verified #22671
Conversation
57e0eda
to
423e812
Compare
212cc66
to
b8edcc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -278,14 +278,12 @@ This endpoint allows you to fetch a specific add-on by id, slug or guid. | |||
line "By Firefox" category | |||
notable Notable category | |||
recommended Recommended category | |||
sponsored Sponsored category | |||
spotlight Spotlight category |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have a look at the addons swagger api documentation and see if the same can be applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the query parameters are documented in swagger according to mozilla/addons#15019
|
The signature changed for AddonQueryParam and AddonQueryMultiParam - they used to be initialized with the query dict (from the request); now they are initialized with the request. We need the request to perform the api_gate check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would think that one push to remove the code references and a separate one to actually clean up the data would be the safer way to do this.
If the migration runs and you delete all the data, but then there is some unseen issue with the code.. well too late.
I can understand not wanting to do that since our current delivery schedule would mean that's a month long process... 🤷
Is this something normal for us? To couple migrations removing data with removing code references in the same patch? @diox do you have an opinion here?
Depend on what the migration is. I would agree we should remove the code first in the cases where the code is responsible for creating that data, and is accessible to the outside world, but in this case it's safe because we know there are no verified/sponsored promotions at the moment (there never was any in prod, as the project was cancelled before going live), and the only way to add any is to have someone with permissions go in the admin to make some. (for schema migrations, we definitely care about backwards-compatibility to allow for rollbacks and just make the deploy painless when we are deploying the new schema with old code still serving requests, and in that case we typically make fields nullable through a migration while removing them from the code, and then remove the column entirely later) |
Fixes: mozilla/addons#14996
Description
Removes sponsored and verified promoted groups. Also add an API gate to silently ignore those promoted group names if passed as a search parameter - currently enabled for v3, v4, and v5, but we could drop v5 once frontend is fixed (as it's explicitly positioned as unstable).
Context
Frontend (and probably autograph) would be separate PRs.
Testing
?promoted=sponsored
paramverified
Checklist
#ISSUENUM
at the top of your PR to an existing open issue in the mozilla/addons repository.